-
Notifications
You must be signed in to change notification settings - Fork 64
Adding e2e make target, makefile adjustments, and e2e test framework #81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: dtfranz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I'm a big fan of getting this setup early so that there's a place for contributors to fairly easily add tests.
One other thing I think we should try to do early on is encourage more unit testing than we did in rukpak for the core functionality. Right now, much of rukpak's functional tests are in e2e's. In e2e's we can't (easily) get coverage metrics, so it's difficult to ensure we're covering critical sections in tests, e2e's generally take an order of magnitude more time to run (making dev cycles take longer way longer), and it's basically impossible to test individual state transitions.
Signed-off-by: dtfranz <[email protected]>
Signed-off-by: dtfranz <[email protected]>
test -s $(LOCALBIN)/kind || GOBIN=$(LOCALBIN) go install sigs.k8s.io/[email protected] | ||
|
||
.PHONY: ginkgo | ||
ginkgo: $(GINKGO) ## Download ginkgo locally if necessary. | ||
$(GINKGO): $(LOCALBIN) | ||
test -s $(LOCALBIN)/ginkgo || GOBIN=$(LOCALBIN) go install github.com/onsi/ginkgo/v2/[email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test -s $(LOCALBIN)/<binName>
tests are sufficient for CI because they'll always fail and we'll always get the correct version.
But locally, these can be annoying when we bump the version of one of these and you still have an old version from an earlier commit. If there's some diff in behavior between your local version and the one specified here, you'll be scratching your head trying to figure out why.
There are two solutions I've seen for this:
- Rukpak has a separate
tools
go module with a separate go.mod that lists all the tool versions with make targets that alwaysgo build
- A somewhat curated script that knows how to check the presence and version of the existing binary to see if a new one is needed called like this. It's mostly set and forget.
Option 1 is good if all of our tools are Go-based
Option 2 is probably slightly faster when actually using the tools, but requires some tweaks every now and then when a tool changes its release artifacts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a follow-up for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did notice number 1 there in rukpak's makefile. I initially was working with that but it ended up turning into a bit of a rabbit-hole with the amount of changes needed, since it appeared as though it would require re-working every built-in make target we got from kubebuilder, and then at that point it felt like I was going to end up with just a copy-paste of rukpak's makefile which would require re-testing all of the targets. We definitely need to take care of it at some point, if you think it's a good thing to take care of now I could give it another go. It just felt like it was getting a bit out-of-scope to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created issue #85 to track this.
} | ||
|
||
var _ = BeforeSuite(func() { | ||
cfg = ctrl.GetConfigOrDie() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these tests go under suite_test.go? Just because we have the test framework already scaffolded by the tool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw Joe's comment #81 (comment), please feel free to ignore my review :)
Only one nit (we could do in a follow up), we could use envtest even for the unit (case 1) tests (though it is mainly intended for integration) if we foresee the interaction of multiple individual components, and would have to test "Reconcile" anyway. This in the case (1) where we need deppy entity source, and essentially mock the functioning of controller. This is just because previously we had faced version incompatibility issues by using locally available binaries and had a hard time figuring the cause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Varsha! I have to admit, my knowledge on the envtest stuff is lacking here, so if it's alright with you I'd like to reserve that for a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, I have created an issue to follow up #86. We can start using envtest when we get to writing tests for reconciler depending on the complexity of those. Till then its good :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this PR, please address Joe's open comments. Looks good otherwise!
kind-load: kind ## Loads the currently constructed image onto the cluster | ||
$(KIND) load docker-image $(IMG) --name $(KIND_CLUSTER_NAME) | ||
|
||
kind-cluster: kind kind-cluster-cleanup ## Standup a kind cluster | ||
$(KIND) create cluster --name ${KIND_CLUSTER_NAME} | ||
$(KIND) export kubeconfig --name ${KIND_CLUSTER_NAME} | ||
|
||
kind-cluster-cleanup: kind ## Delete the kind cluster | ||
$(KIND) delete cluster --name ${KIND_CLUSTER_NAME} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Looking at consistency, we might want to rename these. If we wanted to go with k8s' V-O verbiage:
e.g.
kind-cluster
-> kind-create-cluster
kind-cluster-cleanup
-> kind-delete-cluster
kind-load
-> kind-load-image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your reasoning for these, but I also named them this way for the sake of consistency with rukpak, so we'd need to change them in both places if we also wanted to be internally consistent. If it's alright with you, I'd prefer to leave these at least for now.
/lgtm |
Update README Issue: operator-framework#81 Fix output examples Signed-off-by: Michael Ryan Peter <[email protected]>
Adds the
e2e
target formake
along with the associated github action. Also adjustsrun
to be more in-line with what we do in RukPak, running theoperator-controller
inside a kind cluster instead of the kubebuilder generated default behavior (run locally)To keep the changes to a minimum I've not fully adjusted the makefile to match RukPak's, so there are many functional and stylistic differences which we may want to resolve in the near future.
Signed-off-by: dtfranz [email protected]